Allow to distinguish out of gas from other traps by athei · Pull Request #4883 · paritytech/substrate

athei · 2020-02-10T22:38:10Z

Description
When a contract encounters a runtime error a wasm trap is
triggered and the execution is halted. Currently, no matter
what was the cause for the trap it is always reported as:
DispatchError::Other("contract trapped during execution").

However, the trap that is triggered if a contract exhausts
its gas budget is particulary interesting. Therefore we add
a seperate error message for this cause:
DispatchError::Other("ran out of gas during contract execution").

A test is added hat executes a contract that never terminates.
Therefore it always exhausts is gas budget.

fixes #2584

Open Issues

~~I am not sure whether to bump impl_versionor spec_version.~~

spec_version should be bumped

~~The field of the Other error variant (the error string) seems not to be passed through to the PolkadotAppsUI which renders this improvement rather unsatisfying:~~

This is OK for now

parity-cla-bot · 2020-02-10T22:38:15Z

It looks like @athei hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

athei · 2020-02-10T22:42:18Z

[clabot:check]

parity-cla-bot · 2020-02-10T22:42:20Z

It looks like @athei signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

frame/contracts/src/tests.rs

pepyakin

Good start! I have only a few nits.

frame/contracts/src/tests.rs

pepyakin · 2020-02-11T17:13:12Z

frame/contracts/src/wasm/runtime.rs

 	// - amount: How much gas is used.
 	gas(ctx, amount: u32) => {
-		charge_gas(&mut ctx.gas_meter, ctx.schedule, RuntimeToken::Explicit(amount))?;
+		charge_gas(&mut ctx.gas_meter, ctx.schedule, &mut ctx.special_trap, RuntimeToken::Explicit(amount))?;


This (an a few other) line exceeds the soft limit of 100. It would be great if we wrapped this line, like

charge_gas( &mut ctx.gas_meter, ctx.schedule, &mut ctx.special_trap, RuntimeToken::Explicit(amount) )?;

I will look into the overlong lines.

Removed the long lines I added / mofified.

pepyakin · 2020-02-11T17:13:57Z

frame/contracts/src/wasm/runtime.rs

 enum SpecialTrap {
 	/// Signals that trap was generated in response to call `ext_return` host function.
 	Return(Vec<u8>),
+	OutOfGas,


Would be useful to leave some words about this variant.

Yes. Makes sense.

Added a comment

athei · 2020-02-12T08:49:07Z

Any idea about the Open Issues part in my PR description?

pepyakin · 2020-02-12T09:40:08Z

Ah yeah sorry. Yes, since you change the runtime behavior you need to bump the spec version as well. Otherwise, clients with older native runtime compiled in will execute that stale code rather than the wasm version under some circumstances.

The second concern is valid, but I believe we cannot do anything with it atm.

athei · 2020-02-12T10:40:04Z

The second concern is valid, but I believe we cannot do anything with it atm.

I guess the change would be to the javascript? I do not even see the extrinsic error on the node console. Is this intended or do I change the loglevel? The only way that I see that the change is working is that the test succeeds.

pepyakin · 2020-02-12T14:26:16Z

I think yeah you need to change the log level, try RUST_LOG=runtime=trace.

xlc · 2020-02-13T11:04:14Z

This fixes #2584

When a contract encounters a runtime error a wasm trap is triggered and the execution is halted. Currently, no matter what was the cause for the trap it is always reported as: DispatchError::Other("contract trapped during execution"). However, the trap that is triggered if a contract exhausts its gas budget is particulary interesting. Therefore we add a seperate error message for this cause: DispatchError::Other("ran out of gas during contract execution"). A test is added hat executes a contract that never terminates. Therefore it always exhausts is gas budget.

Remove overlong lines.

Rename Contract to Contracts

* contracts: Allow to distinguish out of gas from other traps When a contract encounters a runtime error a wasm trap is triggered and the execution is halted. Currently, no matter what was the cause for the trap it is always reported as: DispatchError::Other("contract trapped during execution"). However, the trap that is triggered if a contract exhausts its gas budget is particulary interesting. Therefore we add a seperate error message for this cause: DispatchError::Other("ran out of gas during contract execution"). A test is added hat executes a contract that never terminates. Therefore it always exhausts is gas budget. * fixup! contracts: Allow to distinguish out of gas from other traps Remove overlong lines. * fixup! contracts: Allow to distinguish out of gas from other traps Rename Contract to Contracts

athei added A0-please_review Pull request needs code review. M8-contracts labels Feb 10, 2020

athei requested a review from pepyakin February 10, 2020 22:38

athei requested review from Robbepop and gui1117 as code owners February 10, 2020 22:38

NikVolf reviewed Feb 11, 2020

View reviewed changes

frame/contracts/src/tests.rs Outdated Show resolved Hide resolved

pepyakin reviewed Feb 11, 2020

View reviewed changes

athei force-pushed the athei-out-of-gas branch from 0ccaf41 to 943697a Compare February 12, 2020 14:45

paritytech-ci added the B2-breaksapi label Feb 12, 2020

athei force-pushed the athei-out-of-gas branch from 943697a to eb7791c Compare February 12, 2020 14:49

athei requested review from NikVolf and pepyakin February 12, 2020 17:13

athei force-pushed the athei-out-of-gas branch from eb7791c to 8b5df5c Compare February 13, 2020 10:36

NikVolf approved these changes Feb 13, 2020

View reviewed changes

athei added 2 commits February 14, 2020 10:46

fixup! contracts: Allow to distinguish out of gas from other traps

5a99dbe

Remove overlong lines.

athei force-pushed the athei-out-of-gas branch from ee7ec70 to 5a99dbe Compare February 14, 2020 09:47

fixup! contracts: Allow to distinguish out of gas from other traps

58bd3ef

Rename Contract to Contracts

pepyakin merged commit a6e7c05 into master Feb 14, 2020

pepyakin deleted the athei-out-of-gas branch February 14, 2020 10:46

athei mentioned this pull request Sep 27, 2022

Member Request polkadot-fellows/seeding#17

Merged

Conversation

athei commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Feb 10, 2020

Uh oh!

athei commented Feb 10, 2020

Uh oh!

parity-cla-bot commented Feb 10, 2020

Uh oh!

Uh oh!

pepyakin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pepyakin Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

athei Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

athei Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pepyakin Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

athei Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

athei Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

athei commented Feb 12, 2020

Uh oh!

pepyakin commented Feb 12, 2020

Uh oh!

athei commented Feb 12, 2020

Uh oh!

pepyakin commented Feb 12, 2020

Uh oh!

xlc commented Feb 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

athei commented Feb 10, 2020 •

edited

Loading